Skip to content

Rework TypeScript .d.ts handling #978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Rework TypeScript .d.ts handling #978

wants to merge 3 commits into from
+943 −335

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Jul 27, 2025 •
edited
Loading

This reworks .d.ts handling to address the issues in #977 and reported by Are the Types Wrong.

Background information:

  • According to the TypeScript Handbook:

    If you’re using a bundler to emit your library... You must ensure that your declaration files get bundled as well. Recall the first rule of declaration files: every declaration file represents exactly one JavaScript file.

  • According to Are the Types Wrong,

    A golden rule of declaration files is that if they represent a module—that is, if they use import or export at the top level—they must represent exactly one JavaScript file. They especially cannot represent JavaScript files of two different module formats.

  • chartjs-plugin-annotation's CJS build uses a CJS default export (see Rollup's docs). Properly representing that in a .d.ts file requires using export = Annotation, but the export = syntax both raises challenges for exporting types alongside the default export and is incompatible with ESM. (The TypeScript Handbook has even more background information here. See also tshy's documentation, "export default is the bane of hybrid TypeScript modules" - although I believe that the situation is not quite as bad as it says.)

Pulling this together for chartjs-plugin-annotation:

  • Use rollup-plugin-dts to bundle declaration files alongside the bundled library files (as recommended by the TypeScript Handbook).
  • Invoke rollup-plugin-dts twice to create separate .d.ts files for the ESM and CJS builds (and update package.json to reference those).
  • Although a CJS file with a default export can't export additional values, it is apparently possible to export types alongside the default value, if the types are placed in a namespace with the same name as the default value.
  • I had trouble finding a solution for the export = default issue. (I couldn't get rollup-plugin-dts to create an export = style definition. Changing chartjs-plugin-annotation to not use a CJS default export may be the simplest long-term solution, but it would break backward compatibility.) To solve this, I added @rollup/plugin-replace to apply the necessary namespace and export = statement via string replacement. (changed - see below)

Fixes #977

@joshkel
Rework TypeScript .d.ts handling
02be20a 
This reworks .d.ts handling to address the issues in chartjs#977 and reported by [Are the Types Wrong](https://arethetypeswrong.github.io/?p=chartjs-plugin-annotation%403.1.0).

Background information:

* According to the [TypeScript Handbook](https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#im-writing-a-library):

    > If you’re using a bundler to emit your library... You must ensure that your declaration files get bundled as well. Recall the [first rule of declaration files](https://www.typescriptlang.org/docs/handbook/modules/theory.html#the-role-of-declaration-files): every declaration file represents exactly one JavaScript file.

* According to [Are the Types Wrong](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md),

    > A golden rule of declaration files is that if they represent a module—that is, if they use import or export at the top level—they must represent exactly one JavaScript file. They especially cannot represent JavaScript files of two different module formats.

* chartjs-plugin-annotation's CJS build uses a CJS default export (see [Rollup's docs](https://rollupjs.org/configuration-options/#output-exports)). Properly representing that in a `.d.ts` file requires using `export = Annotation`, but the `export =` syntax both raises challenges for exporting types alongside the default export and is incompatible with ESM. (The [TypeScript Handbook](https://www.typescriptlang.org/docs/handbook/modules/appendices/esm-cjs-interop.html) has even more background information here. See also [tshy's documentation](https://github.com/isaacs/tshy?tab=readme-ov-file#handling-default-exports), "`export default`` is the bane of hybrid TypeScript modules" - although I believe that the situation is [not quite as bad as it says](isaacs/tshy#105).)

Pulling this together for chartjs-plugin-annotation:

* Use [rollup-plugin-dts](https://www.npmjs.com/package/rollup-plugin-dts) to bundle declaration files alongside the bundled library files (as recommended by the TypeScript Handbook).
* Invoke rollup-plugin-dts twice to create separate `.d.ts` files for the ESM and CJS builds (and update package.json to reference those).
* Although a CJS file with a default export can't export additional values, it is apparently possible to export types alongside the default value, if the types are placed in a namespace with the same name as the default value.
* I had trouble finding a solution for the `export =` default issue. (I couldn't get rollup-plugin-dts to create an `export =` style definition. Changing chartjs-plugin-annotation to not use a CJS default export may be the simplest long-term solution, but it would break backward compatibility.) To solve this, I added @rollup/plugin-replace to apply the necessary namespace and `export =` statement via string replacement.

Fixes chartjs#977
@joshkel joshkel marked this pull request as draft July 30, 2025 17:10
@joshkel
Copy link
Contributor Author

joshkel commented Jul 30, 2025 •
edited
Loading

I did some further investigation into the export = versus export default issue and realized that my attempted solution didn't work. rollup-plugin-dts's output simply isn't amenable to string replacement like I thought it was. I see a couple of potential paths forward:

  1. Add other code or tooling to generate a .d.cts file with export = and a namespace: Microsoft's API Extractor could probably be made to work with some string replacements, or I could write my own tool (which admittedly is more than I was hoping to tackle 🙂 ).
  2. Give up and use a CJS-ESM-interop style default export: instead of module.export = AnnotationPlugin, do something like exports.default = Annotation; Object.defineProperty(exports, '__esModule', { value: true });. This is a potentially breaking change: anyone who's using the plugin in a CommonJS Node environment will now have to do const AnnotationPlugin = require('chartjs-plugin-annotation').default instead of const AnnotationPlugin = require('chartjs-plugin-annotation'), and anyone using chartjs-plugin-annotation.cjs as a UMD build via <script> tag will be similarly affected, but anyone using the .min.js version or using chartjs-plugin-annotation via bundler (i.e., anyone following the documentation) will be unaffected.
  3. Give up on fixing the errors from Are the Types Wrong: As its docs explain, "the inaccuracy of the typings is not easily observable" (since TypeScript's tsc with the esModuleInterop option and typical bundlers will both cover over the issue).

For the purposes of this PR, I went with approach 2, but please let me know how you'd like to proceed.

If chartjs-plugin-annotation ends up making a breaking change, I wonder if it would be best to separate the CJS and unminified UMD builds, as Chart.js itself does, to make this a little more explicit.

@joshkel joshkel mentioned this pull request Jul 30, 2025
Open
@joshkel
Copy link
Contributor Author

joshkel commented Jul 30, 2025

A fourth approach is to create a separate .d.ts file for the CJS version that re-exports everything. See chartjs/chartjs-plugin-datalabels#436. chartjs-plugin-annotation defines enough data types that this is a decent amount of duplication.

@kurkle
Copy link
Member

kurkle commented Jul 31, 2025

To me, aligning with chart.js style sounds like a good option in the long run. Would not be a huge breaking change, but a breaking change anyway.

@joshkel
Copy link
Contributor Author

joshkel commented Jul 31, 2025

@kurkle , do you want me to pursue that as part of this PR?

@kurkle
Copy link
Member

kurkle commented Jul 31, 2025

@joshkel not quite sure, I'll need to catch up first. @stockiNail, @etimberg thoughts?

@etimberg
Copy link
Member

etimberg commented Aug 1, 2025

Catching up as well. My gut says that we should align with the Chart.js types even if it's breaking now

@stockiNail
Copy link
Collaborator

Apologize for my late reply. Well, I have to admit that during last 2 years I have never touched any javascript code in deep therefore I'm a bit out of the problem. Anyway

To me, aligning with chart.js style sounds like a good option in the long run. Would not be a huge breaking change, but a breaking change anyway.

and

we should align with the Chart.js types even if it's breaking now

I fully agree. Working in the past on chart.js, this plugin and others, I often see this differences, on packaging as well. In my opinion could make sense to align all in chartjs org even if it could be a breaking change.

@joshkel joshkel mentioned this pull request Aug 11, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeScript type issues
4 participants
@joshkel @kurkle @etimberg @stockiNail